Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make [manual_map] ignore types that contain dyn #12712

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

J-ZhengLi
Copy link
Member

fixes: #12659

[manual_map] and [manual_filter] shares the same check logic, but this issue doesn't seems like it could affect manual_filter (?)


changelog: make [manual_map] ignore types that contain dyn

@rustbot
Copy link
Collaborator

rustbot commented Apr 25, 2024

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 25, 2024
@J-ZhengLi J-ZhengLi marked this pull request as draft April 26, 2024 01:33
@dswij dswij added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 4, 2024
@J-ZhengLi J-ZhengLi force-pushed the issue12666 branch 3 times, most recently from f01faac to 00bb73f Compare May 10, 2024 08:23
@J-ZhengLi J-ZhengLi marked this pull request as ready for review May 10, 2024 08:35
@J-ZhengLi
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 10, 2024
@xFrednet
Copy link
Member

Hey, this is a ping from triage. @dswij can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using r? clippy.

@rustbot ready

@Alexendoo
Copy link
Member

I'll take this one since I looked into it previously, r? me

@rustbot
Copy link
Collaborator

rustbot commented Jun 21, 2024

Failed to set assignee to me: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@Alexendoo Alexendoo assigned Alexendoo and unassigned dswij Jun 21, 2024
Comment on lines 78 to 80
if some_expr.expr_contains_ty_adjustments(cx) && expr_has_explicit_ty(cx, expr) {
return None;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expr_has_explicit_ty misses some other coercion sites such as

fn main() {
    let x: &[u8; 4] = b"1234";
    f(match Some(()) {
        Some(_) => Some(Box::new(x)),
        None => None,
    });
}

fn f(_: Option<Box<&[u8]>>) {}

I think an easier way to rule out unrelated coercion false negatives would be to recursively check the coercion propagating subexpressions (see the second list in the above link) rather than every subexpression for adjustments

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, shame to admit that I'm currently stuck on this 🤦‍♂️.

I'm pretty sure I misunderstand the concept, and I couldn't get it to work, especially for this specific test case:

//~v ERROR: manual implementation of `Option::map`
let _ = match Some(0) {
    Some(_) => Some(match f() {
        Ok(res) => Ok(Box::new(res)),
        _ => Err(()),
    }),
    None => None,
};

Do you mind provid some more guidence? 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into it more I was incorrect earlier, I had assumed that the point at which the adjustment happens wouldn't cross "past" a coercion site, i.e. in

let x: &[u8; 4] = b"1234";
f(match Some(()) {
    Some(_) => Some(Box::new(x)),
               ~~~~~~~~~~~~~~~~~ I thought the adjustment happened here
                             ~ but it happens here
    None => None,
});

I think checking for coercion propagating expressions and expressions that are coercion sites may still work though, roughly

fn is_coerced(expr) -> bool {
    if expr has adjustments {
        return true;
    }

    match expr.kind {
        // regular coercion sites
        Call | MethodCall => args.any(is_coerced),
        Struct | Enum | Union => fields.any(is_coerced),
        // propagating
        Array => elements.any(is_coerced),
        Repeat => is_coerced(repeated),
        Tuple => fields.any(is_coerced),
        Block => is_coerced(block.expr),
        If => is_coerced(then) || is_coerced(r#else),
        Match => arms.any(is_coerced),
    }
}

The key difference to the current implementation is the direction, we're looking at children rather than parents. In your example the adjustment that we don't care about happens at f, but we avoid a false negative because we never check that expression

let _ = match Some(0) {
    Some(_) => Some(match f() {
                          ~ adjustment here
        Ok(res) => Ok(Box::new(res)),
        _ => Err(()),
    }),
    None => None,
};

There will be false positives though:

fn g(b: &[u8]) -> Box<&[u8]> {
    Box::new(b)
}

let x: &[u8; 4] = b"1234";

// ok
let _: Option<Box<&[u8]>> = match Some(0) {
    Some(_) => Some(g(x)),
    None => None,
};

// also ok
let _: Option<Box<&[u8]>> = Some(0).map(|_| g(x));

You might be able to eliminate these by checking that the type of the relevant arg/field/etc is concrete, but also accepting those FNs is perfectly fine to me

Copy link
Member Author

@J-ZhengLi J-ZhengLi Jun 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an interesting FN case in manual_map_option.rs:

match Some(&String::new()) {
    Some(x) => Some(str::len(x)),
    None => None,
};

the &String was coerced to &str but it shouldn't matter right? I don't know if checking the type for concrete concreteness is possible, thus I'm still finding a way to eliminate this 😸

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a Ty I think it would be something like .has_type_flags(TypeFlags::HAS_TY_PARAM)

You can use expr_sig on the callee/receiver as like a tcx.fn_sig that works for closures/etc too

If the output of the function doesn't have a ty param then it should(?) be fine to return false

For individual args you could zip the inputs with the args, only checking the arg if it HAS_TY_PARAM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works perfectly! Thank you so much 👍

@J-ZhengLi J-ZhengLi force-pushed the issue12666 branch 2 times, most recently from ceec8fa to a03f382 Compare June 25, 2024 10:29
@J-ZhengLi
Copy link
Member Author

J-ZhengLi commented Jun 29, 2024

I need more time on this

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 29, 2024
@J-ZhengLi J-ZhengLi force-pushed the issue12666 branch 2 times, most recently from 765401a to b386427 Compare July 1, 2024 07:24
@J-ZhengLi J-ZhengLi removed the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Jul 2, 2024
@J-ZhengLi J-ZhengLi added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

manual_map sugg with dyn causes error
5 participants